nixos/thermald: add manual config generation #44923

Vodurden · 2018-08-12T04:32:40Z

Motivation for this change

thermald has two modes: zero-config and manual. Sometimes it is useful to manually configure thermald to achieve better thermal results or to give thermald a hand when detecting possible cooling options.

I have tested this change on my own laptop running NixOS and observed significant improvements in the thermals of my laptop.

With this change manual thermald config can be encoded in configuration.nix. For example:

  services.thermald = {
    enable = true;
    configFile.platforms = [{
      Name = "CPU passive cooling limit to 80C";
      ProductName = "*";
      Preference = "QUIET";
      ThermalZones = [{
        Type = "x86_pkg_temp";
        TripPoints = [{
          SensorType = "x86_pkg_temp";
          Type = "passive";
          Temperature = 80000;
          CoolingDevices = [{
            Index = 1;
            Type = "rapl_controller";
            Influence = 50;
            SamplingPeriod = 10;
          } {
            Index = 2;
            Type = "intel_pstate";
            Influence = 40;
            SamplingPeriod = 10;
          } {
            Index = 3;
            Type = "intel_powerclamp";
            Influence = 30;
            SamplingPeriod = 10;
          } {
            Index = 4;
            Type = "cpufreq";
            Influence = 20;
            SamplingPeriod = 8;
          } {
            Index = 5;
            Type = "Processor";
            Influence = 10;
            SamplingPeriod = 5;
          }];
        }];
      }];
    }];
  };

I'm not sure if the naming is exactly what we want, particularly around configFile.platforms so I'd appreciate any input on how to make this more consistent with other NixOS modules.

I'd also like some feedback on how to better implement TripPoints.CoolingDevices. At the moment I've implemented CoolingDevices as a list but in the actual XML it simply allows for multiple CoolingDevice entries under TripPoints.

Currently we do:

TripPoints = [{
  ...
  CoolingDevices = [{
    ...
  }, {
    ...
  }];
}];

But if we want to mirror how the XML is actually represented it would be closer to do:

TripPoints = [{
  ...
  CoolingDevice = { ... };
  CoolingDevice = { ... };
}];

But I'm unsure how to get submodules to merge correctly given the above syntax. Any input would be appreciated :)

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

infinisil · 2018-08-13T02:26:52Z

I'm not sure if it's worth adding almost 40 options for the small number of people that will actually use them. NixOS evaluation time is still about linear in the amount of options, and as long as that's not resolved I think it would be wise not to use options for every single thing. Although, probably in the long term we shouldn't refrain from adding lots of options for things. But we have no idea how long it will be until somebody comes up with a solution to the evaluation time problem (I've been thinking about it myself for a while now, I have an idea, but no implementation yet).

As an alternative, see how I use a single option to represent the whole config in #41467, and then use this to generate a syntactically valid config file. This has the advantage of only using one option and not needing NixOS module adjustments with new versions, but the disadvantage of not being able to check for valid config settings with the module system. What still can be done is to use the package itself to check for valid config settings at build time (by running it with some config check option, if supported).

@Vodurden What do you think about this? I'm fine with either adding all the options or doing it the alternate way. I'll start reviewing the code after this is cleared up.

Vodurden · 2018-08-13T05:44:34Z

@infinisil: Ooh that's an interesting approach, if I understand it correctly you're building a small abstract syntax tree using nix's semantic types and using it to represent the configuration file which you then compile into the config.

I'll give it a shot and see how the resulting code turns out. Though it is a shame not to be able to use the options system to it's fullest as I think we lose some of the documentation discoverability which thermald definitely needs.

Vodurden · 2018-08-13T06:14:56Z

I just had a quick look into this: thermald doesn't seem to have a config file verification mode so I won't be able to use exactly the same approach and maintain config file checking.

With that in mind it seems like we're evaluating two options

Use nixos options

Pros:

Current state, no extra work for me
Full documentation for all options
Static checking for config
Consistent with the long-term approach we want to take for nixos config generation?

Cons:

Will negatively impact nixos evaluation time due to a bug that may take a long time to be fixed.

Use a generic semantic type and compile it to config

This is the newly suggested option

Pros:

No impact on nixos evaluation time
Doesn't require updates to the module if new config is added to thermald

Cons:

Config cannot be checked for validity unless I do something like writing a DTD for thermald. (thermald doesn't have a config checker).
No documentation for config options

Use a specific semantic type and compile it to config

This option is the same idea as above except I could encode a more specific type system that matches the allowable config in thermald. (I haven't tried this option so I'm not sure if it will actually work but it feels theoretically possible)

~~Pros:~~

~~- No impact on nixos evaluation time~~
~~- Config can be checked for validity~~

~~Cons:~~

~~- No documentation for config options~~
~~- Requires updates if thermald config changes~~

I had a look at this approach and realised I don't think I can implement it without basically re-implementing submodules!

What to do?

Overall I'm not sure what approach to take. I'm not a big fan of introducing a generic config syntax tree since it won't have any static checking and will fail at runtime if the config is incorrect. But I also don't want to slow down the evaluation time for everything.

Ultimately I guess what I need to know is:

Is the slowdown to nixos evaluation significant enough to be worth doing a bunch of work?
Would we be happy with the semantic type config approach even if the results can't be validated?

Given that is can't be validated my preference is to keep the current approach but I'm open to trying alternatives if that's not palatable :)

infinisil · 2018-08-13T13:02:35Z

Neat overview! So I now think that doing it the current way is probably better. The point that convinced me is that docs for the available options are really nice, especially because thermald's config file docs are kinda bad. The NixOS module can serve as a better replacement for them.

Very nice first PR btw! I'll try to review the code today :)

infinisil · 2018-08-13T16:10:26Z

nixos/modules/services/hardware/thermald.nix

+  platformConfig = with types; submodule {
+    options = {
+      Name = mkOption {
+        type = string;


All strings should be strs instead, see

nixpkgs/lib/types.nix

Lines 210 to 212 in 249ba3d

# Deprecated; should not be used because it quietly concatenates

# strings, which is usually not what you want.

string = separatedString "";

infinisil · 2018-08-13T16:10:48Z

nixos/modules/services/hardware/thermald.nix

+    options = {
+      Name = mkOption {
+        type = string;
+        description = "The name of this platform";


All descriptions should end with a .

infinisil · 2018-08-13T16:12:25Z

nixos/modules/services/hardware/thermald.nix

+      };
+
+      ThermalSensors = mkOption {
+        type = (listOf thermalSensorConfig);


Parens can be removed (applies to lots of other options as well)

infinisil · 2018-08-13T16:20:28Z

nixos/modules/services/hardware/thermald.nix

+
+      Temperature = mkOption {
+        type = int;
+        description = "Temperature at which to take action";


Would be good to include the unit, which apparently is millidegree celsius after some digging in the source.

infinisil · 2018-08-13T16:24:01Z

nixos/modules/services/hardware/thermald.nix

+      };
+
+      ControlType = mkOption {
+        type = nullOr (enum [ "SEQUENTIAL" "PARALLEL" ]);


From the source I see that it doesn't matter whether these settings are upper-cased or lower-cased. So I think we should prefer to use a consistent casing (Type above uses lower-cased). Not sure why they use this mix in the man page. I'd prefer lower-casing.

infinisil · 2018-08-13T17:22:17Z

nixos/modules/services/hardware/thermald.nix

+      script = ''
+        exec ${pkgs.thermald}/sbin/thermald \
+          --no-daemon \
+          ${optionalString cfg.debug "--loglevel=debug"} \


This can be put into serviceConfig.ExecStart directly, without the exec

infinisil · 2018-08-13T17:24:39Z

nixos/modules/services/hardware/thermald.nix

+        '';
+      };
+
+      configFile = mkOption {


I think it would be better to name this option config and introduce a new option configFile, which will default to the generated config, but will then be overridable by the user. Then cfg.configFile can also be passed directly to the exec script and to etc.

infinisil · 2018-08-13T17:27:09Z

nixos/modules/services/hardware/thermald.nix

+
+  platformConfig = with types; submodule {
+    options = {
+      Name = mkOption {


Options are named according to camelCase, so all of the first letters should be lower cased instead (except UUID).

Would we keep UUID as is? Or should it be uuid?

Oh actually a couple options seem to be using uuid, so that's probably better

infinisil · 2018-08-13T17:28:51Z

nixos/modules/services/hardware/thermald.nix

+      Name = mkOption {
+        type = string;
+        description = "The name of this platform";
+      };


A lot of options could use example values

infinisil · 2018-08-13T17:31:10Z

nixos/modules/services/hardware/thermald.nix

+
+            ki = mkOption {
+              type = string;
+              description = "ki";


Also a lot of options could use better descriptions, e.g. these ones. But it might be pretty hard to find out what these exactly represent.

I'd love to give these better descriptions but I have no idea what they do. The documentation is pretty sparse on them and I'm assuming if you want to set them then you know what they are 👀

With that said if anyone knows what they do I'm happy to include a better description!

Vodurden · 2018-08-14T09:45:33Z

Thanks for doing such a detailed review! I've addressed the documentation issues as best I can. I'll follow up with some more changes in the next few days to address the XML generation and configuration naming issues.

edolstra · 2018-08-14T10:13:50Z

I'm reluctant about this. @infinisil already brought up the evaluation performance issue: the NixOS evaluation model is currently not very scalable, so adding dozens of service-specific options should be avoided.

The other argument is (lack of) maintainability: we're basically replicating the entire thermald configuration language in NixOS, so every time thermald adds an option, we'd have to update the NixOS module to make it available to users. We also have to duplicate all option descriptions from the upstream documentation, which is also not very nice - it's better to just refer to the upstream docs.

Vodurden · 2018-08-14T10:21:28Z

I think they're reasonable concerns but I am having a hard time deciphering what guidelines I should be following in situations like this. When I asked about this in irc it seemed like the preference for this sort of situation is to embed the configuration in the nix language, but now it seems like we'd prefer to just embed the configuration files?

I'm happy to follow either approach but I'd really appreciate some clear and consistent guidelines around how to enable this sort of service configuration in nixos.

It's probably worth noting that the upstream docs are fairly sparse and difficult to decipher but I guess that could be solved with some documentation PRs to thermald rather then doing it here in the option configuration. 🤔

Vodurden · 2018-08-18T05:12:20Z

Ok! So in the interest of getting this merged I have removed the nixos option based configuration in favor of the proposed configFile attribute. This allows configuration like so:

  services.thermald = {
    enable = true;
    configFile = builtins.toFile "thermal-conf.xml" ''
      <?xml version="1.0"?>
      <ThermalConfiguration>
        <Platform>
          <Name>Keep CPU below 80C</Name>
          <ProductName>*</ProductName>
          <Preference>quiet</Preference>

          <ThermalZones>
            <ThermalZone>
              <Type>x86_pkg_temp</Type>
              <TripPoints>
                <TripPoint>
                  <SensorType>x86_pkg_temp</SensorType>
                  <Type>passive</Type>
                  <Temperature>80000</Temperature>

                  <CoolingDevice>
                    <Index>1</Index>
                    <Type>rapl_controller</Type>
                    <Influence>50</Influence>
                    <SamplingPeriod>10</SamplingPeriod>
                  </CoolingDevice>

                  <CoolingDevice>
                    <Index>2</Index>
                    <Type>intel_pstate</Type>
                    <Influence>40</Influence>
                    <SamplingPeriod>10</SamplingPeriod>
                  </CoolingDevice>

                  <CoolingDevice>
                    <Index>3</Index>
                    <Type>intel_powerclamp</Type>
                    <Influence>30</Influence>
                    <SamplingPeriod>10</SamplingPeriod>
                  </CoolingDevice>

                  <CoolingDevice>
                    <Index>4</Index>
                    <Type>cpufreq</Type>
                    <Influence>20</Influence>
                    <SamplingPeriod>8</SamplingPeriod>
                  </CoolingDevice>

                  <CoolingDevice>
                    <Index>5</Index>
                    <Type>Processor</Type>
                    <Influence>10</Influence>
                    <SamplingPeriod>5</SamplingPeriod>
                  </CoolingDevice>
                </TripPoint>
              </TripPoints>
            </ThermalZone>
          </ThermalZones>
        </Platform>
      </ThermalConfiguration>
    '';
  };

I have separated the option based configuration into a separate commit available here: Vodurden@494d997

If we decide to merge the follow-up commit it would be backwards compatible with this change as it simply sets the default of configFile to the generated config as suggested by @infinisil.

My feeling is that hopefully it's not too controversial to merge the configFile based approach, if there is a great desire for the option based approach I'm happy to open a follow-up PR that introduces it :)

I've tested this configFile approach and confirmed that it has the same behavior as the option based approach on my machine.

@infinisil, @edolstra: Please let me know if there are any other concerns/blockers for getting this change merged and I'll do my best to resolve them as soon as I can!

infinisil

Yeah I think this is in the right direction, it's very common to have such an option for services. It can still be expanded to more powerful options later.

infinisil · 2018-08-18T14:20:28Z

nixos/modules/services/hardware/thermald.nix

+      configFile = mkOption {
+        type = types.nullOr types.str;
+        default = null;
+        description = "the thermald manual configuration file.";


There are 2 common options for modules in NixOS:

configFile, type nullOr path, represents the path to the config file

extraConfig, type lines, represents the contents of the config file, will be appended to a generated one.

Since it doesn't make much sense to have extraConfig here (because appending to valid xml makes it invalid), I think it might be best to just have the configFile option with the path type.

It's still easily possible to convert a string to a path by doing

services.thermald.configFile = builtins.toFile "thermald.conf" '' <foo> ... </foo> ''

infinisil · 2018-08-18T14:22:03Z

nixos/modules/services/hardware/thermald.nix

+          ${pkgs.thermald}/sbin/thermald \
+            --no-daemon \
+            ${optionalString cfg.debug "--loglevel=debug"} \
+            ${optionalString (cfg.configFile != null) "--config-file ${config.environment.etc."thermald/thermal-conf.xml".source.outPath}"} \


With the above mentioned change, this would also become an easier --config-file ${cfg.configFile}

Vodurden · 2018-08-19T05:33:19Z

@infinisil: Using the path type makes perfect sense, I've updated this PR with the requested changes. Thanks!

Let me know if there's anything else blocking this PR from being merged

thermald has two modes: zero-config and manual. Sometimes it is useful to manually configure thermald to achieve better thermal results or to give thermald a hand when detecting possible cooling options.

infinisil

LGTM!

samueldr requested a review from infinisil August 12, 2018 23:56

infinisil requested changes Aug 13, 2018

View reviewed changes

Vodurden force-pushed the add-thermald-config branch from 2ebaf00 to 6b35529 Compare August 14, 2018 09:44

Vodurden force-pushed the add-thermald-config branch from 6b35529 to 9182bb1 Compare August 18, 2018 05:08

infinisil requested changes Aug 18, 2018

View reviewed changes

Vodurden force-pushed the add-thermald-config branch from 9182bb1 to 3b036e2 Compare August 19, 2018 05:29

nixos/thermald: add manual config file

64223a2

thermald has two modes: zero-config and manual. Sometimes it is useful to manually configure thermald to achieve better thermal results or to give thermald a hand when detecting possible cooling options.

Vodurden force-pushed the add-thermald-config branch from 3b036e2 to 64223a2 Compare August 19, 2018 05:54

infinisil approved these changes Aug 19, 2018

View reviewed changes

samueldr merged commit 730b8b1 into NixOS:master Aug 19, 2018

Vodurden deleted the add-thermald-config branch March 16, 2021 04:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/thermald: add manual config generation #44923

nixos/thermald: add manual config generation #44923

Vodurden commented Aug 12, 2018 •

edited

Loading

infinisil commented Aug 13, 2018

Vodurden commented Aug 13, 2018

Vodurden commented Aug 13, 2018 •

edited

Loading

infinisil commented Aug 13, 2018

infinisil Aug 13, 2018

infinisil Aug 13, 2018

infinisil Aug 13, 2018

infinisil Aug 13, 2018

infinisil Aug 13, 2018

infinisil Aug 13, 2018

infinisil Aug 13, 2018

infinisil Aug 13, 2018

Vodurden Aug 14, 2018

infinisil Aug 14, 2018

infinisil Aug 13, 2018

infinisil Aug 13, 2018

Vodurden Aug 14, 2018

Vodurden commented Aug 14, 2018

edolstra commented Aug 14, 2018

Vodurden commented Aug 14, 2018

Vodurden commented Aug 18, 2018 •

edited

Loading

infinisil left a comment

infinisil Aug 18, 2018

infinisil Aug 18, 2018

Vodurden commented Aug 19, 2018

infinisil left a comment

	# Deprecated; should not be used because it quietly concatenates
	# strings, which is usually not what you want.
	string = separatedString "";

nixos/thermald: add manual config generation #44923

nixos/thermald: add manual config generation #44923

Conversation

Vodurden commented Aug 12, 2018 • edited Loading

Motivation for this change

Things done

infinisil commented Aug 13, 2018

Vodurden commented Aug 13, 2018

Vodurden commented Aug 13, 2018 • edited Loading

Use nixos options

Use a generic semantic type and compile it to config

Use a specific semantic type and compile it to config

What to do?

infinisil commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vodurden commented Aug 14, 2018

edolstra commented Aug 14, 2018

Vodurden commented Aug 14, 2018

Vodurden commented Aug 18, 2018 • edited Loading

infinisil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vodurden commented Aug 19, 2018

infinisil left a comment

Choose a reason for hiding this comment

Vodurden commented Aug 12, 2018 •

edited

Loading

Vodurden commented Aug 13, 2018 •

edited

Loading

Vodurden commented Aug 18, 2018 •

edited

Loading